fix: Impose whole-directory size limit; apply 18h limit to pending dir#321
fix: Impose whole-directory size limit; apply 18h limit to pending dir#321awforsythe wants to merge 9 commits into
Conversation
refs: RUM-16880
This PR addresses a few gaps in storage-thread functionality that could
lead to runaway disk usage in certain situations, such as when a the
tracking consent value never changes from `pending`.
### Problems
Compared to the mobile SDKs, the C++ SDK has the following limitations:
1. Age-based file eviction (for files older than 18h) only runs on the
granted-consent directory; there's no such mechanism bounding the
growth of the pending-consent directory.
2. The C++ SDK imposes no whole-directory disk usage limitation- on
mobile platforms, no single feature's event storage directory is
allowed to exceed 512 MB.
3. Less critically: whereas the mobile SDKs impose a limit on the
maximum size of a single event (no event can exceed 512 kB on
Android; 1MB on iOS), the C++ SDK only applies a 4MB limit to the
entire batch
The combination of issues 1 and 2 means that current versions of the C++
SDK will allow batches of event data stored in the pending-consent
directory to remain on disk, causing overall disk usage to grow
unbounded.
### Fixes
This PR addresses all three of these problems. Key code changes:
- `IFilesystem` now supports `GetFileSize()`, implemented via `stat()`
or `GetFileAttributesExW()`
- In the upload thread, when actually processing batch files for upload,
we still reject (and delete) files older than 18h, just as we did
before- applying the 18h limit to the consent-granted directory is a
natural side effect of the ordinary file age check on upload.
- We now _also_ apply the 18h limit to the consent-pending directory, as
an explicit check that's run on a per-feature basis, also in the
upload thread, every time an upload cycle is invoked for that feature,
regardless of whether anything was uploaded.
- In the storage thread, when we're preparing a brand new batch file for
write, we now stat all existing batch files in the directory (ignoring
files with non-numeric names entirely), check if they sum to greater
than 512 MB, and iteratively delete files (oldest-first) until we're
back under that limit.
- In the storage thread, when we write any event, we now enforce a
maximum per-event size limit of 512 kB.
As a result, when the SDK is operated normally with the same set of
features registered, it will maintain an overall disk usage footprint no
greater than `512 MB * N`, where N is the number of features registered.
Refactoring changes:
- In `upload_thread.cpp`, we now check file age in two separate code
paths (normal upload and explicit pending-dir eviction check), so we
use a new `batch_file_age()` helper to compute the age of a file given
its filename and the current time
- In `storage_write.cpp`, when we need to cut a new batch file, we
preemptively list the names of all files in the directory we're about
to write to. Previously, this list of filenames was just used to
prevent conflicts when naming the new file. As of this PR, we _also_
need to use this list of filenames when performing the size-based
eviction check. As a result, our directory listing (via
`CacheKnownFilenames()`) has been pulled up to a higher scope, and we
rely on the fact that `_last_known_filenames` is pre-populated in both
of those lower-level routines:
- `CacheKnownFilenames()` runs first, populating the set of
filenames, filtering it to only numerically-named files, and
sorting it
- `PurgeDirectoryIfNeeded()` runs next, stat'ing all files in that
list; if it deletes old files; it removes them from the front of
the list, mutating `_last_known_filenames`
- `GetFilenameForNextWrite()` runs next, using
`_last_known_filenames` as-is (so we only fetch the directory
listing once per new batch file creation)
Test changes:
- `MockFilesystem` now supports `GetFileSize()`, including targeted mock
failures
- `storage_write_test.cpp` now validates:
- Single event exceeding max event size is dropped
- Single event exactly matching max event size is accepted
- If batch dir exceeds directory size limit on write, old files are
purged
- If batch dir is within size limit on write, no files are purged
- If foreign (non-SDK-written) files exist in batch dir, they are
ignored on purge
- Both pending and granted directories are subject to file size
limit and will be purged the same
- Filesystem errors in size checks are handled gracefully
- `upload_thread_test.cpp` now validates:
- If a pending dir has files older than 18h when an upload cycle
runs, those files are deleted
- If all files in a pending dir are newer than 18h, the pendir dir
is left untouched
- Foreign files older than 18h are left alone, never deleted
- If we fail to list files at the start of an eviction check, we
skip the age-based eviction process entirely
|
@codex review |
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7b04a8f4fb
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
This ensures that an early-out on the upload cycle due to failure to list files in the granted directory will not prevent deletion of outdated files in the pending directory. We still run the pending-directory eviction check once per upload cycle; it just runs at the start of the cycle (before upload attempts) rather than at the end (after upload attempts).
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 917306cb0c
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| continue; | ||
| } | ||
|
|
||
| const auto delete_res = fsw.Delete(pending_file_path.CStr()); |
There was a problem hiding this comment.
Avoid racing pending eviction with consent migration
When consent is granted while this upload-cycle cleanup is deleting an expired pending file, the storage thread can have already listed that same file in FeatureEventStorage::MigratePendingBatchesToGranted; if this Delete wins the race, the subsequent Rename sees DoesNotExist, falls into the migration path's non-AlreadyExists error case, and returns false, aborting migration for the rest of the pending directory. That strands any younger pending batches after the deleted file even though consent is now granted, so they are never uploaded (and will later be aged out by this cleanup).
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
There is indeed a race here, and we have to deal with it by design:
- The upload thread is periodically running age-based file eviction in the pending-consent directory
- The storage thread may simultaneously delete or rename files from that directory on tracking consent change
This is largely fine, because the only shared resource is the filesystem: maintaining the correct behavior is just a matter of handling filesystem errors gracefully.
If the upload thread loses this race (storage thread renames or deletes a file before the upload thread can delete it), then we already handle this correctly: for any error on pending-batch deletion, the upload thread just logs a warning and continues.
However, this review note raises a legitimate issue in the case where the storage thread loses the race - i.e. when the upload thread successfully deletes a file before the storage thread can carry out its rename on migration. In this case, the storage thread will fail to rename the file (with a DoesNotExist error) and will abort migration, causing the remaining pending files (if any exist) to be left behind in the pending directory. Those files will sit around until they age out at 18 hours, denying them the opportunity to be uploaded.
This is an uncommon edge case, but it can be fixed fairly simply: the storage thread can just tolerate DoesNotExist (under the assumption that another thread deleted the file) and continue with its migration.
Fixed: 640075f
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 640075f578
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2f352646eb
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
b16b2db to
a7c1c2e
Compare
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a7c1c2e814
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bbb7e73321
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1cd562f9e0
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // The file was removed by another agent (quota purge or age eviction) between | ||
| // the directory listing and this open. Treat it as a consumed batch so the | ||
| // upload cycle continues rather than backing off. | ||
| return _process_and_upload_batch_result::bad_batch; |
There was a problem hiding this comment.
Treat raced-away missing batches as neutral
When the file disappears between the granted-directory listing and OpenForRead, this branch returns bad_batch. The caller still increments num_uploads_attempted, attempts a second delete, and, if this missing entry is the only or last batch in the cycle, reaches the final non-success path that increases the feature backoff. That turns the quota or age eviction race this code is trying to tolerate into a delayed next upload cycle; return a neutral/skipped result or avoid counting it as an attempted upload instead.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
The only code that can delete files from the granted directory between listing and open is the storage thread's quota purge, and that requires the 512 MB threshold to be exceeded and a new batch file created in the window between the directory listing and the open() call — a nanosecond-wide window in practice. If it does fire, the consequence is a one-step backoff increase for one feature in one cycle, which the next successful upload resets. Not worth complicating the upload loop for.
| // If the rename failed because the source file no longer exists, the upload | ||
| // thread's age-based eviction has already removed it. The file was expired and | ||
| // would not have been uploaded; skip it and continue migrating the remaining files. | ||
| if (rename_res == FilesystemResult::DoesNotExist) { |
There was a problem hiding this comment.
Verify the source is missing before skipping rename failures
DoesNotExist from Rename is not specific to the source file; the filesystem maps missing path components such as the granted directory to the same result. If v1/ is removed after initialization while pending files remain, consent-grant migration now reports success and leaves those files stranded in intermediate-v1/, and future granted writes will still fail because the destination directory is gone. Confirm the source was evicted before continuing, or surface the rename error.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
v1/ is created by Initialize() and nothing in the SDK ever removes it- the only way it disappears is external modification of the storage directory. Even in that hypothetical, the failure mode is milder than described: since v1/ doesn't exist, the upload thread has nothing to upload from either, so the pending files sit in intermediate-v1/ rather than being lost, and migration succeeds on the next Initialize() when the directory is recreated.
refs: RUM-16880
This PR addresses a few gaps in storage-thread functionality that could lead to runaway disk usage in certain situations, such as when a the tracking consent value never changes from
pending.Problems
Compared to the mobile SDKs, the C++ SDK has the following limitations:
Age-based file eviction (for files older than 18h) only runs on the granted-consent directory; there's no such mechanism bounding the growth of the pending-consent directory.
The C++ SDK imposes no whole-directory disk usage limitation- on mobile platforms, no single feature's event storage directory is allowed to exceed 512 MB.
Less critically: whereas the mobile SDKs impose a limit on the maximum size of a single event (no event can exceed 512 kB on Android; 1MB on iOS), the C++ SDK only applies a 4MB limit to the entire batch
The combination of issues 1 and 2 means that current versions of the C++ SDK will allow batches of event data stored in the pending-consent directory to remain on disk, causing overall disk usage to grow unbounded.
Fixes
This PR addresses all three of these problems. Key code changes:
IFilesystemnow supportsGetFileSize(), implemented viastat()orGetFileAttributesExW()In the upload thread, when actually processing batch files for upload, we still reject (and delete) files older than 18h, just as we did before- applying the 18h limit to the consent-granted directory is a natural side effect of the ordinary file age check on upload.
We now also apply the 18h limit to the consent-pending directory, as an explicit check that's run on a per-feature basis, also in the upload thread, every time an upload cycle is invoked for that feature, regardless of whether anything was uploaded.
In the storage thread, when we're preparing a brand new batch file for write, we now stat all existing batch files in the directory (ignoring files with non-numeric names entirely), check if they sum to greater than 512 MB, and iteratively delete files (oldest-first) until we're back under that limit.
In the storage thread, when we write any event, we now enforce a maximum per-event size limit of 512 kB.
As a result, when the SDK is operated normally with the same set of features registered, it will maintain an overall disk usage footprint no greater than
512 MB * N, where N is the number of features registered.Refactoring changes:
In
upload_thread.cpp, we now check file age in two separate code paths (normal upload and explicit pending-dir eviction check), so we use a newbatch_file_age()helper to compute the age of a file given its filename and the current timeIn
storage_write.cpp, when we need to cut a new batch file, we preemptively list the names of all files in the directory we're about to write to. Previously, this list of filenames was just used to prevent conflicts when naming the new file. As of this PR, we also need to use this list of filenames when performing the size-based eviction check. As a result, our directory listing (viaCacheKnownFilenames()) has been pulled up to a higher scope, and we rely on the fact that_last_known_filenamesis pre-populated in both of those lower-level routines:CacheKnownFilenames()runs first, populating the set of filenames, filtering it to only numerically-named files, and sorting itPurgeDirectoryIfNeeded()runs next, stat'ing all files in that list; if it deletes old files; it removes them from the front of the list, mutating_last_known_filenamesGetFilenameForNextWrite()runs next, using_last_known_filenamesas-is (so we only fetch the directory listing once per new batch file creation)Test changes:
MockFilesystemnow supportsGetFileSize(), including targeted mock failuresstorage_write_test.cppnow validates:upload_thread_test.cppnow validates: